use Win32 file APIs and widePath for file ops to support long filenames #1004
Conversation
daveinglis
requested review from
dmbryson,
jakepetroules and
owenv
as code owners
|
@swift-ci test |
daveinglis
changed the title
| } | ||
| if (RemoveDirectoryW(wpath.data())) { | ||
| return 0; | ||
| } else { |
There was a problem hiding this comment.
Unnecessary } else {. Just use } and the rest of the body can follow.
daveinglis
force-pushed
the
windows_long_file_fix
branch
from
2a26b43 to
e0c6ead
Compare
|
@swift-ci test |
lib/Basic/PlatformUtility.cpp
Outdated
| llvm::SmallVector<wchar_t, MAX_PATH> wPath; | ||
| if (llvm::sys::path::widenPath(path, wPath)) | ||
| return std::string(); | ||
| CreateDirectoryW(wPath.data(), NULL); |
There was a problem hiding this comment.
Does it make sense to log the error?
There was a problem hiding this comment.
I don't see a lot of logging... Are you thinking just a fprintf maybe?
There was a problem hiding this comment.
Yeah ... at least we would have something to go on if it ever failed.
daveinglis
force-pushed
the
windows_long_file_fix
branch
from
e0c6ead to
3ff8d69
Compare
|
@swift-ci test |
daveinglis
force-pushed
the
windows_long_file_fix
branch
from
3ff8d69 to
c9736f5
Compare
|
@swift-ci test |
daveinglis
force-pushed
the
windows_long_file_fix
branch
from
c9736f5 to
bb4eef8
Compare
|
@swift-ci test |
|
@swift-ci please smoke test |
| #else | ||
| return ::rmdir(path); | ||
| #endif | ||
| } | ||
|
|
||
| int sys::stat(const char *fileName, StatStruct *buf) { | ||
| #if defined(_WIN32) | ||
| return ::_stat(fileName, buf); |
There was a problem hiding this comment.
@compnerd Would it be simpler to use _wstat, _wrmdir, etc., vs the Win32 functions? They support long paths too, as they are just wrappers around Win32 anyways.
There was a problem hiding this comment.
so there would need some char to wchar_t conversion here to use those, so Im not sure it saves that much since the widePath API does this already.
There was a problem hiding this comment.
I think you might've misunderstood what I meant. I was suggesting that instead of writing thirty lines of code, you simply call:
llvm::SmallVector<wchar_t, MAX_PATH> wfilename;
if (llvm::sys::path::widenPath(fileName, wfilename)) {
errno = EINVAL;
return -1;
}
return ::_wstat(wfilename, buf);...as opposed to GetFileAttributesExW and all the conversion ceremony that follows. I'm also concerned this is subtly changing behavior as you're filling in the stat info differently from how _stat and _wstat do it, now.
I think you should:
- Switch CreateDirectoryW to _wmkdir
- Switch RemoveDirectoryW to _wrmdir
- Switch GetFileAttributesExW to _wstat
- Switch DeleteFileW to _wunlink
That will ensure error handling and behavior is consistent with previously.
There was a problem hiding this comment.
do these _w functions accept the //?/ prefixed paths?
There was a problem hiding this comment.
The Microsoft implementation does support the NT style paths I believe but they are disjoint from Win32. e.g. usingSetEnvironmentVariableW and wgetenv would give unexpected results. I think that using win32 might unify some behaviors.
There was a problem hiding this comment.
do these _w functions accept the //?/ prefixed paths?
Like I said, they just wrap the underlying Win32 functions with a little ceremony on top. So by definition, yes. e.g. the implementation of _wmkdir literally just calls CreateDirectoryW and does nothing but that other than set errno to an appropriate value based on GetLastError.
ucrt's source is also available in the Windows SDK and you can inspect it.
e.g. usingSetEnvironmentVariableW and wgetenv would give unexpected results.
Environment is a little different than the other functions.
|
It seems like the windows-toolchain build failed after this commit. https://ci-external.swift.org/job/swift-main-windows-toolchain/1526 Looking at the commits that were part of the first failing run. This seemed like the most likely culprit given that it modifies a function called Pull request testing also seems blocked e.g https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-windows/44356/ |
|
I mentioned this in the revert, but the issue here is that PR testing was testing tip rather than the PR itself. I'll edit this comment once that's fixed. EDIT: That should be fixed now. Worth checking the commit in the next PR run though 😅 |
No description provided.